Feat: new analytics privacy controller #7643
Conversation
…ad of dates - Changed `deleteRegulationDate` to `deleteRegulationTimestamp` in the state and related methods to store timestamps in milliseconds since epoch. - Updated relevant methods and tests to reflect the new timestamp format. - Removed date formatting logic and adjusted selectors accordingly. - Added new dependencies for testing and updated the test suite to ensure proper functionality with the new timestamp format. This change enhances consistency in handling date-related data within the analytics privacy controller.
NicolasMassart
added
enhancement
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…e PascalCase - Refactored enum values in `DataDeleteResponseStatus` and `DataDeleteStatus` to follow PascalCase naming convention. - Updated all references in the codebase and tests to ensure consistency with the new enum values. - This change enhances code readability and aligns with common TypeScript practices.
…st title - Rename Error to Failure (and Ok to Success) for clearer naming - Fix duplicate test title in AnalyticsPrivacyController.test.ts - Update all references across the codebase
…tead of type-only
There was a problem hiding this comment.
Pull request overview
This PR introduces a new @metamask/analytics-privacy-controller package that provides GDPR/CCPA data deletion functionality for analytics data, extracting logic from the mobile app for reuse across MetaMask clients.
Changes:
- New controller package with
AnalyticsPrivacyControllerfor managing data deletion state (tracksdataRecorded,deleteRegulationId, anddeleteRegulationTimestamp) AnalyticsPrivacyServicefor communicating with Segment's Regulations API via proxy endpoint with retry/circuit-breaker logic- Comprehensive test coverage (100% branch, function, line, and statement coverage) and selector utilities for state access
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/analytics-privacy-controller/src/AnalyticsPrivacyController.ts | Main controller implementation managing deletion request lifecycle and state |
| packages/analytics-privacy-controller/src/AnalyticsPrivacyService.ts | Service for HTTP communication with Segment Regulations API via proxy |
| packages/analytics-privacy-controller/src/types.ts | Type definitions for deletion statuses and API responses |
| packages/analytics-privacy-controller/src/selectors.ts | Reusable state selectors |
| packages/analytics-privacy-controller/src/constants.ts | Segment API constants |
| packages/analytics-privacy-controller/package.json | Package configuration with dependencies |
| packages/analytics-privacy-controller/*.test.ts | Comprehensive test suites for controller and service |
| yarn.lock | Lockfile updates for new package dependencies |
| tsconfig.json, tsconfig.build.json | TypeScript project references |
| teams.json, .github/CODEOWNERS | Team ownership configuration |
| README.md | Documentation updates including package in list and dependency graph |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
# Conflicts: # yarn.lock
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Hey @NicolasMassart, sorry for the late review, I had some questions/comments below.
| this.#messenger = options.messenger; | ||
| this.#fetch = options.fetch; | ||
| this.#segmentSourceId = options.segmentSourceId; | ||
| this.#segmentRegulationsEndpoint = options.segmentRegulationsEndpoint; |
There was a problem hiding this comment.
My thought around data services is that they should represent an API, and the implication is that the exact API they represent should be obvious. However, it seems that this class accepts any URL. That means the knowledge about which external system this is talking to is somewhere else.
Is it possible to include the URL in this same file, perhaps as a constant? If it's based on environment, perhaps the class can take an environment and use that to build the URL?
There was a problem hiding this comment.
Nit: Since there is no class here, is there a reason why this file needs to be PascalCase? Thoughts on calling this file logger.ts?
| /** | ||
| * Indicates if data has been recorded since the last deletion request. | ||
| */ | ||
| dataRecorded: boolean; |
There was a problem hiding this comment.
In checkDataDeleteStatus, this is renamed to hasCollectedDataSinceDeletionRequest. I think this name does a good job of describing the purpose. Thoughts on renaming the state property?
| dataRecorded: boolean; | |
| hasCollectedDataSinceDeletionRequest: boolean; |
| const { analyticsId } = analyticsControllerState; | ||
|
|
||
| if (!analyticsId || analyticsId.trim() === '') { | ||
| const error = new Error('Analytics ID not found'); |
There was a problem hiding this comment.
Nit: Do we want a more descriptive message which instructs the engineer on how to fix this? Perhaps something like "You need to set up AnalyticsController with an analytics ID. You can do this by..." (etc.)
|
|
||
| if ( | ||
| response.status === DataDeleteResponseStatus.Success && | ||
| response.regulateId && |
There was a problem hiding this comment.
We seem to be validating the response data. Is there a reason why we are doing that here? Ideally, the service should return data that is usable, and if not, should throw an error.
| * Status values for data deletion requests from Segment API. | ||
| * Enum values match Segment API response values exactly. | ||
| */ | ||
| export enum DataDeleteStatus { |
There was a problem hiding this comment.
What are your thoughts on using a type object rather than an enum? We are considering banning enums in core (and other places) because they have a lot of issues, one of which is that two enums with the same name and same content aren't assignable to each other (nominal typing), which causes unexpected type errors. You can read more here: MetaMask/eslint-config#417
For instance, you could replace this enum with:
export enum DATA_DELETE_STATUSES = {
Failed: 'FAILED',
Finished: 'FINISHED',
Initialized: 'INITIALIZED',
Invalid: 'INVALID',
NotSupported: 'NOT_SUPPORTED',
PartialSuccess: 'PARTIAL_SUCCESS',
Running: 'RUNNING',
Unknown: 'UNKNOWN',
} as const;
export type DataDeleteStatus =
(typeof DataDeleteStatus)[keyof typeof DataDeleteStatus];| /** | ||
| * Regulation ID from Segment API. | ||
| */ | ||
| export type DataDeleteRegulationId = string | undefined; |
There was a problem hiding this comment.
Can an ID be undefined? That seems interesting 🤔
| } catch (error) { | ||
| log('Analytics Deletion Task Error', error); | ||
| return { | ||
| status: DataDeleteResponseStatus.Failure, |
There was a problem hiding this comment.
Same question I posed in the controller — is there a reason why throwing an error would not achieve the same thing? Or in this case, since the service is supposed to be lower-level, is there a reason why it's useful to catch errors at all? Why not let them bubble up?
There was a problem hiding this comment.
Nit: These constants seem to only be used in the service. Would it be easier to maintain if they lived there?
| const dataDeleteStatus = Object.values(DataDeleteStatus).includes( | ||
| rawStatus as DataDeleteStatus, | ||
| ) | ||
| ? (rawStatus as DataDeleteStatus) | ||
| : DataDeleteStatus.Unknown; |
There was a problem hiding this comment.
Nit: If you had an isDataDeleteStatus function then you could simplify this and wouldn't need to use typecasting:
function isDataDeleteStatus(status: unknown): status is DataDeleteStatus {
const dataDeleteStatuses: string[] = Object.values(DataDeleteStatus);
return dataDeleteStatuses.includes(status);
}| const dataDeleteStatus = Object.values(DataDeleteStatus).includes( | |
| rawStatus as DataDeleteStatus, | |
| ) | |
| ? (rawStatus as DataDeleteStatus) | |
| : DataDeleteStatus.Unknown; | |
| const dataDeleteStatus = isDataDeleteStatus(rawStatus) | |
| ? rawStatus | |
| : DataDeleteStatus.Unknown; |
Explanation
This PR introduces a new
@metamask/analytics-privacy-controllerpackage that provides GDPR/CCPA data deletion functionality for analytics data. The package allows to extract the logic from the mobile app (and will be compatible with extension too)Current state: MetaMask mobile app currently has a dedicated mechanism to handle user data deletion requests for analytics data in compliance with GDPR and CCPA regulations.
Solution: This package introduces:
dataRecordedflag,deleteRegulationId, anddeleteRegulationTimestampto support compliance workflowsImplementation details:
AnalyticsController:getStateto retrieve the user's analytics IDAnalyticsPrivacyServiceto make HTTP requests to Segment's Regulations APIcreateServicePolicyfrom@metamask/controller-utilsfor retry logic and error handlingReferences
see also MetaMask/metamask-mobile#22016
Fixes #7618
Checklist
Note
Adds a new package
@metamask/analytics-privacy-controllerfor analytics GDPR/CCPA data deletion workflows.AnalyticsPrivacyController(state:dataRecorded,deleteRegulationId,deleteRegulationTimestamp; methods:createDataDeletionTask,checkDataDeleteStatus, getters, and flag update; events for task creation/state changes)AnalyticsPrivacyService(proxy HTTP calls to Segment Regulations API; retry/circuit-breaker viacreateServicePolicy; exposed actions for create/check)Written by Cursor Bugbot for commit 2664bfb. This will update automatically on new commits. Configure here.